Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for support Instance::of in configuration #21

Merged
merged 3 commits into from
Dec 21, 2020

Conversation

antonovsky
Copy link
Contributor

Using createObject with \yii\di\Instance::of in configuration does not work.
Creating an instance of the application in the suggested way solves the problem.

@SamMousa
Copy link
Collaborator

SamMousa commented Nov 18, 2020

This breaks instantiation using the DI container, therefore it is a BC break.
What issue is this PR fixing? Do you have an issue number?

@SamMousa SamMousa self-requested a review November 18, 2020 10:22
@antonovsky
Copy link
Contributor Author

I didn't create an issue. I posted this PR instead. Should I create an issue?

@SamMousa
Copy link
Collaborator

No but some more explanation in here would be nice. This change breaks some existing behaviour so it needs to be motivated. Can it be fixed in another way without breaking instantiation via the container?

@antonovsky
Copy link
Contributor Author

Why you think this breaks instantiation using the DI container? What do you mean? The tests in my project are still working correctly. DI work.

@samdark samdark added the bug Something isn't working label Nov 18, 2020
@SamMousa
Copy link
Collaborator

Why you think this breaks instantiation using the DI container? What do you mean?

I'm sorry I was unclear.
It breaks instantiation of the application object itself via the DI container, because it no longer uses the DI container.

Your issue can actually be solved by putting this in the DI config, this wouldn't require any change to the module code.

\yii\web\Application::class => static function(Container $c, array $params): \yii\web\Application {
    return new \yii\web\Application($params);
}

@antonovsky
Copy link
Contributor Author

antonovsky commented Nov 18, 2020

My app config looks something like this. Did you mean this?
If I place your code in singletons or definitions, the error does not disappear:
Fatal error: Uncaught ReflectionException: Class MyComponentClassFromDI does not exist in /vendor/yiisoft/yii2/di/Container.php on line 449

[
    'container' => [
        'singletons' => [
            // singletons
            \yii\web\Application::class => static function(Container $c, array $params): \yii\web\Application {
                return new \yii\web\Application($params);
            },
            'MyComponentClassFromDI' => [
                'class' => app\components\MyComponentClassFromDI::class,
            ],
        ],

        'definitions' => [
            // definitions
        ],
    ],
    'components' => [
        'MyComponentClassFromDI' => \yii\di\Instance::of('MyComponentClassFromDI'),
    ],
  // other application configuration things
];

@antonovsky
Copy link
Contributor Author

I could be wrong, but i think at the time of the creation of the application itself, the DI container does not exist at all. Hence the problem with Instance::of().

@SamMousa
Copy link
Collaborator

That depends on what is in your bootstrap.
I create the container before anything else, the container is then used to create the application object.

So that is what will break with your change.

@antonovsky
Copy link
Contributor Author

Thank you very much. If done in the way you suggested, everything will work.
Last question: Why not let the application itself do the instantiation of the container?

@SamMousa
Copy link
Collaborator

This is subjective, others might not share my opinion.

In my view the container consists outside the application and the method of configuring it via app config array is just a dirty hack so people can have all configuration in one place. https://github.com/yiisoft/yii2/blob/master/framework/Yii.php this file creates it as a side effect.

I don't use that file but instead use my own bootstrap that instantiates the container. Imo that's cleaner

@antonovsky
Copy link
Contributor Author

@SamMousa , How do you like this new revision? I think it works for all options.

@SamMousa
Copy link
Collaborator

It doesn't, I think you've misunderstood my issue.

My DI container config tells the container how to create the application. So any creation that uses new Application will not take that into account.

@antonovsky
Copy link
Contributor Author

@SamMousa , Have you looked at the new code? The new version uses createObject instead of new Application.
That is, instead of changing the way the application object was created, I added Yii::configure(Yii::$container, $config['container']);

@SamMousa
Copy link
Collaborator

Hmm, github shows just 1 file changed... and that is only the Yii::configure stuff... let me see if I can find a different diff...

@SamMousa
Copy link
Collaborator

Ah wait, now I get it, you have reverted your old change so this is the full feature.

Yes, that looks good to me!

You take out container config and use that directly; feels like a clean solution to me!

@antonovsky
Copy link
Contributor Author

Yes, i change only 1 file (src/Codeception/Lib/Connector/Yii2.php):

        if (isset($config['container']))
        {
            Yii::configure(Yii::$container, $config['container']);
            unset($config['container']);
        }

@samdark
Copy link
Member

samdark commented Nov 23, 2020

Looks alright but we need to fix tests before merge to ensure it doesn't break things: #22

Copy link
Member

@Naktibalda Naktibalda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase on master to get the tests passing.

Using createObject with \yii\di\Instance::of in configuration does not work.
Creating an instance of the application in the suggested way solves the problem.
Using createObject with \yii\di\Instance::of in configuration does not work.
Creating an instance of the application in the suggested way solves the problem.
@antonovsky
Copy link
Contributor Author

Please rebase on master to get the tests passing.

done

@samdark samdark merged commit ac59fe6 into Codeception:master Dec 21, 2020
@samdark
Copy link
Member

samdark commented Dec 21, 2020

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants